Cache derive proc macro expansion with incremental query #145354
+258
−49
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
S-waiting-on-review
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
rustbot
added
the
T-compiler
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Aug 13, 2025
Cache derive proc macro expansion with incremental query
rustbot
added
the
S-waiting-on-perf
3 tasks
bjorn3
reviewed
Aug 13, 2025
| } | ||
| } | ||
|
|
||
| impl<H: std::hash::Hasher> SpanEncoder for HashEncoder<H> { |
There was a problem hiding this comment.
Shouldn't this use StableHasher? The regular Hash impl is only valid for a single rustc session.
There was a problem hiding this comment.
This is something that wasn't answered in #129102 (comment).
There was a problem hiding this comment.
Hmm, I need to read up on the difference of usage of Hash and StableHash for incremental. I examined a few other query key types and a lot of them just #[derive(Hash)]. I did the same for TokenStream, but would appreciate guidance on if it is the Right Thing to do :)
bjorn3
reviewed
Aug 13, 2025
bjorn3
reviewed
Aug 13, 2025
| /// Must be called while the `enter` function is active. | ||
| fn with<F, R>(f: F) -> R | ||
| where | ||
| F: for<'a, 'b> FnOnce(&'b mut ExtCtxt<'a>, DeriveClient) -> R, |
There was a problem hiding this comment.
The ExtCtxt contains a bunch of things that would need to be tracked by the query system for sound caching. And the rest could either be retrieved from the tcx or be created from scratch to avoid having to use a thread local to bypass the query system.
There was a problem hiding this comment.
This was discussed a bit in #129102 (comment) and a few comments right above it.
petrochenkov
removed
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 21e746d failed: CI. Failed jobs:
|
This comment has been minimized.
This comment has been minimized.
|
@bors try |
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Aug 13, 2025
Cache derive proc macro expansion with incremental query
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for b712f62 failed: CI. Failed jobs:
|
cjgillot
reviewed
Aug 13, 2025
Comment on lines
+148
to
+149
| let macro_def_id = invoc_expn_data.macro_def_id.unwrap(); | ||
| let proc_macro_crate_hash = tcx.crate_hash(macro_def_id.krate); |
There was a problem hiding this comment.
This can be done from inside the query, to force a dependency on crate_hash, not as part of the key.
There was a problem hiding this comment.
Sorry, my knowledge of the query infrastructure is quite limited 😅 How would that work? The key would be just LocalExpnId and &TokenStream, and then inside the query I would just call tcx.crate_hash and throw away the results? 🤔
|
I looked into the benchmark failure on diesel, seems like I hold queries wrong somehow :) Any suggestions on what could be the case? Maybe the derived |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a revival of #129102, originally implemented by @futile. Since it looks like they are not active currently, I'd like to push this work forward.
The first commit is squashed and rebased work from the original PR, with author attribution to futile. The rest of the commits are some additional comments that I created mostly for myself to understand what happens here. I also did some cleanups based on Vadim's review comments on the original PR, plus I refactored the TLS access a bit using
scoped_tls.The biggest issue, as usually, are tests... I tried using
#[rustc_clean(..., loaded_from_disk = "derive_macro_expansion")], but the problem is that since this query cannot recover the original key from its hash, and thus its fingerprintstyle isFingerprintStyle::Opaque, this crashes when I try to useloaded_from_disk. Any suggestions from someone who actually understands the query system would be welcome 😅To answer one review question from the original PR: the
Hashimplementation forTokenStreamis indeed called, and it is needed since the stream forms a part of a query key. Not sure about theEncoderHashimplementation though.The last commit is WIP to enable the functionality by default for rustc-perf, I'll do another perf. run here.
TODO: document the new unstable flag
r? @petrochenkov